Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor simple example to typescript #1975

Merged
merged 10 commits into from
Oct 20, 2022

Conversation

belgattitude
Copy link
Contributor

@belgattitude belgattitude commented Oct 20, 2022

While working on esm publication: #1973, I'd like to ensure this works with esmExternals and compute potential size differences.

To do so:

  • I moved the simple example to typescript (cause next move their examples too, more popular nowadays)
  • Added a way to inject experimental.esmExternals through env.
  • Added a step in CI for it

I took the existing example as base. It think the config is a bit outdated. I'll comment in the file change

props: {
...await serverSideTranslations(locale, ['common', 'footer']),
...await serverSideTranslations(locale ?? 'en', ['common', 'footer']),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? Shouldn't it fallback automatically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not the case, we may move this check down here and fallback to defaultLocale if undefined.

Copy link
Contributor Author

@belgattitude belgattitude Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

locale is always there (except maybe for nextjs prior to when locale was added)

I don't really want to the throw Error in there. I feel it's enough with the ??.

Second for the sake of the simple example I don't really want to require/import the next-i18next.config.js to get the fallback. At least not yet, cause it wasn't working in olders version without the localePath trick. I mean all those next-i18next, nextjs legacy but supported. At least If I remember well.

That said a more advanced example should be great.

One that typechecks the keys and handle properly localePath in all scenarios (getStatic, getServer, app and csr pages + monorepo), but for that the example will look a bit like bryanprimus/layout-reproduce-i18next#1. Which will make the simple example less simple.

So for now, I'm just focusing on whatever help me for publishing in a safe way.

Doing so I saw a lot more to do in the codebase and those little might not be necessary anymore once I've cleaned'up. (not sure I can but let's hope)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean in the library code, not in the example code... check the links in my previous comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tldr; I would remove the ?? 'en' part, as this should already work and if not, we'll adapt the library code to automatically fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. To be sure we're talking about the same

Here nextjs type the locale as string | undefined, and I want it as string.

Even if we would accept an undefined as first parameter to serverSideTranslations and do the job there, I feel would worsen the signature (I mean having optional as first param is not living well, ie myFunction(undefined, undefined, {param: 1})

I don't feel neither than nextjs is at fault here. It's good that the developer handles the fallback. I mean in how next-i18next works right now. And mysefl I tend to create my own function (that will deal with more)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@adrai
Copy link
Member

adrai commented Oct 20, 2022

image

@belgattitude
Copy link
Contributor Author

belgattitude commented Oct 20, 2022

If there's no blocking comment and CI passes, I feel this could be merged.

On my side it looks good

@@ -43,3 +43,7 @@ jobs:
- name: E2E tests
run: npm run test:e2e

- name: Build example with experimental.esmExternals
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the part that I want to merge into the peer-deps change and new publishing. It will help me cover edge cases

@adrai adrai merged commit c22f9b6 into i18next:master Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants